-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move reset to DisplayModeTrait #126
Conversation
The functionality is common, so the implementation can be as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, but I disagree about the changelog. It's sort of-kinda-not breaking, but might break things for people that don't like using preludes. I'd suggest adding something like the following to the Changed section:
- [#126](https://github.com/jamwaffles/ssd1306/pull/126) Moved `reset` method to `DisplayModeTrait`. If the prelude is not used, add either `use ssd1306::prelude::*` or `ssd1306::mode::displaymode::DisplayModeTrait` to your imports.
Fair enough. I also marked it as breaking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last nit then this can be merged :)
Co-authored-by: James Waples <james@wapl.es>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for bearing with me
No worries, thanks for accepting my contributions :) |
Any time! |
The functionality is common, so the implementation can be as well.
Hi! Thank you for helping out with SSD1306 development! Please:
master
if you're not already up to dateCHANGELOG.md
entry in the Unreleased section under the appropriate heading (Added, Fixed, Changed, etc)rustfmt
on the project withcargo fmt --all
- CI will not pass without this stepPR description
Just a bit of cleanup. Should be backwards compatible since the trait is in the prelude. I don't think there should be a changelog entry for this one.